Skip to content

[offload] Add missing build dependency #149326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jprotze
Copy link
Collaborator

@jprotze jprotze commented Jul 17, 2025

libc++ headers must be generated before compiling part of liboffload.
The build error occurs if clang is configured to use libc++ by default.
Fixes issue #149324

libc++ headers must be generated before compiling part of liboffload.
The build error occurs if clang is configured to use libc++ by default.
Fixes issue llvm#149324
@llvmbot
Copy link
Member

llvmbot commented Jul 17, 2025

@llvm/pr-subscribers-offload

Author: Joachim (jprotze)

Changes

libc++ headers must be generated before compiling part of liboffload.
The build error occurs if clang is configured to use libc++ by default.
Fixes issue #149324


Full diff: https://github.com/llvm/llvm-project/pull/149326.diff

1 Files Affected:

  • (modified) offload/tools/offload-tblgen/CMakeLists.txt (+4)
diff --git a/offload/tools/offload-tblgen/CMakeLists.txt b/offload/tools/offload-tblgen/CMakeLists.txt
index 15525dc44ea60..64562fc72feac 100644
--- a/offload/tools/offload-tblgen/CMakeLists.txt
+++ b/offload/tools/offload-tblgen/CMakeLists.txt
@@ -22,5 +22,9 @@ add_tablegen(offload-tblgen OFFLOAD
   RecordTypes.hpp
   )
 
+if(TARGET cxx-headers)
+  add_dependencies(offload-tblgen cxx-headers)
+endif()
+
 set(OFFLOAD_TABLEGEN_EXE "${OFFLOAD_TABLEGEN_EXE}" CACHE INTERNAL "")
 set(OFFLOAD_TABLEGEN_TARGET "${OFFLOAD_TABLEGEN_TARGET}" CACHE INTERNAL "")

@jprotze jprotze added the openmp:libomptarget OpenMP offload runtime label Jul 17, 2025
@callumfare
Copy link
Contributor

This change looks fine, but is there a reason it only affects offload-tblgen? Could other offload components also run into this issue? I'm wondering if it's possible to capture the dependency at the level of the entire runtime when multiple are being built (e.g. openmp and offload might depend on libcxx)

@jprotze
Copy link
Collaborator Author

jprotze commented Jul 17, 2025

This change looks fine, but is there a reason it only affects offload-tblgen? Could other offload components also run into this issue? I'm wondering if it's possible to capture the dependency at the level of the entire runtime when multiple are being built (e.g. openmp and offload might depend on libcxx)

I was also a bit surprised, that adding this specific dependency fixes the build reproducibly (I did multiple clean builds with 100 processes). I think, that simply all other liboffload code depends on offload-tblgen ?

libomp is not affected, because we carefully avoid dependencies to a C++ runtime library.

@jhuber6
Copy link
Contributor

jhuber6 commented Jul 17, 2025

So, this is for bootstapping with libcxx headers? It makes sense but i am wondering why it only shows up here.

@jprotze
Copy link
Collaborator Author

jprotze commented Jul 17, 2025

I'm not completely sure, what you mean with bootstrapping here, but I would say: No!

It affects building the whole llvm-project, even when using a llvm build from the same sources (built without offload). The problem is that for compiling the runtimes, we use the clang from the outer build directory, but it doesn't have the includes available at this point.

I tried to understand the dependencies in the ninja build file. I think that all other targets in offload have a dependency on some inc files that are processes with offload-tblgen and therefore have a dependency on offload-tblgen.

@jprotze
Copy link
Collaborator Author

jprotze commented Jul 18, 2025

I just tested, whether I can use a more general dependency and tried add_dependencies(LLVMOffload cxx-headers) + add_dependencies(omptarget cxx-headers) while removing the add_dependencies(offload-tblgen cxx-headers) dependency.
With that change, I get the same missing header issues as before.

Empirically, I would say this dependency is both necessary and sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
offload openmp:libomptarget OpenMP offload runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants